-
Notifications
You must be signed in to change notification settings - Fork 292
[ROB-2999] Alertmanager support for gcp #1990
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
WalkthroughNew documentation adding a Google Managed Alertmanager integration guide and updating related Prometheus/metric-provider docs to reference it, including sample YAMLs, OperatorConfig guidance, and verification steps for webhook delivery. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: Organization UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
✏️ Tip: You can disable this entire section by setting Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
✅ Docker image ready for
Use this tag to pull the image for testing. 📋 Copy commandsgcloud auth configure-docker us-central1-docker.pkg.dev
docker pull us-central1-docker.pkg.dev/robusta-development/temporary-builds/robusta-runner:629dd5c
docker tag us-central1-docker.pkg.dev/robusta-development/temporary-builds/robusta-runner:629dd5c me-west1-docker.pkg.dev/robusta-development/development/robusta-runner-dev:629dd5c
docker push me-west1-docker.pkg.dev/robusta-development/development/robusta-runner-dev:629dd5cPatch Helm values in one line: helm upgrade --install robusta robusta/robusta \
--reuse-values \
--set runner.image=me-west1-docker.pkg.dev/robusta-development/development/robusta-runner-dev:629dd5c |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@docs/configuration/alertmanager-integration/google-managed-alertmanager.rst`:
- Around line 106-108: The Alertmanager integration docs show a curl example
using "Authorization: Bearer <ACCOUNT_ID> <SIGNING_KEY>" but the Alertmanager
YAML snippet uses authorization.credentials without the "Bearer" prefix; update
the documentation to either (a) change the Alertmanager secret/config example so
authorization.credentials explicitly includes the Bearer prefix (e.g.,
credentials: 'Bearer <ACCOUNT_ID> <SIGNING_KEY>') or (b) add a clarifying
sentence next to http_config.authorization / authorization.credentials stating
that the full header must include the "Bearer " prefix to match the curl
example, ensuring consistency between the YAML example and the curl validation
snippet.
- Around line 36-40: The authorization block is missing the Bearer token type
which causes the Authorization header to omit the required "Bearer" prefix;
update the authorization mapping by either adding a new key "type: Bearer"
alongside the existing "credentials" field or by prefixing the credentials value
with "Bearer " (i.e., change credentials to "Bearer <ACCOUNT_ID>
<SIGNING_KEY>"), ensuring the "authorization" block contains either type: Bearer
+ credentials or credentials starting with "Bearer ".
🧹 Nitpick comments (2)
docs/configuration/alertmanager-integration/google-managed-alertmanager.rst (2)
62-71: Clarify which externalLabels are optional.The example includes
locationandproject_idin addition to the requiredcluster_namelabel. Consider adding a comment in the YAML or a note indicating which fields are optional versus required, to help users customize the configuration appropriately.📝 Suggested documentation enhancement
collection: externalLabels: - cluster_name: gmp-demo-cluster # Match your Robusta cluster_name, or use "external" - location: us-central1-c - project_id: my-gcp-project + cluster_name: gmp-demo-cluster # Required: Match your Robusta cluster_name, or use "external" + location: us-central1-c # Optional: GCP location for organizational purposes + project_id: my-gcp-project # Optional: GCP project identifier rules: externalLabels: - cluster_name: gmp-demo-cluster - location: us-central1-c - project_id: my-gcp-project + cluster_name: gmp-demo-cluster # Required: Must match the collection cluster_name + location: us-central1-c # Optional: Should match collection if specified + project_id: my-gcp-project # Optional: Should match collection if specified
128-135: Consider clarifying the purpose of duplicate cluster labels.The test payload includes both
cluster_name: "external"andcluster: "external"labels. If both are necessary for compatibility or different purposes, consider adding a comment explaining why both are included.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
docs/configuration/alertmanager-integration/google-managed-alertmanager.rstdocs/configuration/alertmanager-integration/google-managed-prometheus.rstdocs/configuration/index.rstdocs/configuration/metric-providers-google.rst
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
- GitHub Check: run_tests
- GitHub Check: run_tests
- GitHub Check: build
- GitHub Check: Deploy docs
- GitHub Check: Deploy docs
🔇 Additional comments (5)
docs/configuration/alertmanager-integration/google-managed-alertmanager.rst (2)
73-97: Well-structured demo alert.The test alert using
vector(1)will fire immediately and continuously, making it ideal for verifying the webhook integration. The annotations clearly explain its purpose and remind users to clean it up.
8-8: Verify the "external" cluster_name value is documented and supported.The documentation references using
"external"as a cluster_name value for alerts that don't belong to a specific runner. Please confirm that this is an officially supported/reserved value in Robusta's codebase (check cluster_name validation logic, configuration schemas, and any relevant code handling) and ensure this usage is clearly documented with examples so users understand when to use "external" versus a custom cluster name.docs/configuration/index.rst (1)
49-52: LGTM - Navigation link added correctly.The new grid item for Google Managed Alertmanager follows the established pattern and correctly links to the new documentation page.
docs/configuration/alertmanager-integration/google-managed-prometheus.rst (1)
12-14: Good cross-reference to the new guide.The note appropriately directs users to the dedicated Google Managed Alertmanager guide, helping them find the correct documentation for their use case.
docs/configuration/metric-providers-google.rst (1)
96-97: Clear guidance between recommended and legacy options.Good job distinguishing the recommended Google Managed Alertmanager integration from the legacy guide. This helps users make the right choice for their setup.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
No description provided.